Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a login throttling to firewalls(アカウントロック、ログイン回数制限、ログイン試行回数制限、スロットリング) #5473

Merged
merged 4 commits into from
Aug 1, 2022
Merged

Add a login throttling to firewalls(アカウントロック、ログイン回数制限、ログイン試行回数制限、スロットリング) #5473

merged 4 commits into from
Aug 1, 2022

Conversation

pineray
Copy link
Contributor

@pineray pineray commented Jul 26, 2022

概要(Overview・Refs Issue)

ログインに指定回数失敗すると、一定期間ログイン制限を行う機能を追加する。
#4249

方針(Policy)

Symfony 5.2以降は標準でログイン試行回数を制限する機能が存在するため、これを使用する。

相談（Discussion）

chihiro-adachi さんの提案では .env にパラメーターを設定して、それを security.yaml へ渡すという仕様でしたが、そのようにすると max_attempts の値が not numeric value のエラーとなるため、security.yaml へ直接記述するようにしています。

マイナーバージョン互換性保持のための制限事項チェックリスト

  • 既存機能の仕様変更はありません
  • フックポイントの呼び出しタイミングの変更はありません
  • フックポイントのパラメータの削除・データ型の変更はありません
  • twigファイルに渡しているパラメータの削除・データ型の変更はありません
  • Serviceクラスの公開関数の、引数の削除・データ型の変更はありません
  • 入出力ファイル(CSVなど)のフォーマット変更はありません

レビュワー確認項目

  • 動作確認
  • コードレビュー
  • E2E/Unit テスト確認(テストの追加・変更が必要かどうか)
  • 互換性が保持されているか
  • セキュリティ上の問題がないか
    • 権限を超えた操作が可能にならないか
    • 不要なファイルアップロードがないか
    • 外部へ公開されるファイルや機能の追加ではないか
    • テンプレートでのエスケープ漏れがないか

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2022 •
edited
Loading

Codecov Report

Merging #5473 (087d284) into 4.2 (9bafbe4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                4.2    #5473      +/-   ##
============================================
+ Coverage     78.74%   78.78%   +0.03%     
+ Complexity     6251     6246       -5     
============================================
  Files           468      468              
  Lines         21007    20982      -25     
============================================
- Hits          16543    16530      -13     
+ Misses         4464     4452      -12
Flag Coverage Δ
E2E 64.61% <100.00%> (+0.28%) ⬆️
Unit 77.45% <100.00%> (+0.46%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Eccube/EventListener/LoginHistoryListener.php 93.33% <100.00%> (+0.47%) ⬆️
src/Eccube/EventListener/SecurityListener.php 84.00% <0.00%> (-12.00%) ⬇️
src/Eccube/Log/Processor/TokenProcessor.php 90.00% <0.00%> (-10.00%) ⬇️
src/Eccube/Request/Context.php 96.15% <0.00%> (-3.85%) ⬇️
src/Eccube/Controller/Block/CalendarController.php 93.75% <0.00%> (-1.57%) ⬇️
src/Eccube/Entity/TaxRule.php 85.18% <0.00%> (-1.24%) ⬇️
src/Eccube/Service/OrderHelper.php 95.45% <0.00%> (-0.57%) ⬇️
src/Eccube/Service/CartService.php 94.78% <0.00%> (-0.48%) ⬇️
src/Eccube/Service/MailService.php 90.34% <0.00%> (ø)
src/Eccube/Form/Type/Install/Step3Type.php 100.00% <0.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bafbe4...087d284. Read the comment docs.

@nanasess
Copy link
Contributor

nanasess commented Jul 26, 2022 •
edited
Loading

テスト用のプラグインインストール時に以下のようなシステムエラーが発生している模様です

Login throttling is not supported when security.enable_authenticator_manager is not set to true.

https://github.com/EC-CUBE/ec-cube/blob/4.2/codeception/_data/plugins/Bundle-1.0.0.tgz をインストールしようとすると再現します

@nanasess
Bundle プラグインの修正
5cae973 
services.yaml の設定を以下に変更

```
security:
  enable_authenticator_manager: true
```
@nanasess
Copy link
Contributor

ストアのプラグインインストール時に以下のようなシステムエラーが発生している

プラグインのテストで使用している Bundle プラグインの security.enable_authenticator_manager: false になっていたのが原因のようです。 true に変更してみました

@nanasess nanasess marked this pull request as ready for review July 26, 2022 14:17
@nanasess
Copy link
Contributor

@pineray
テスト通りましたので、ドラフト外しました！

@pineray
Copy link
Contributor Author

pineray commented Jul 27, 2022

@nanasess
サポートいただきありがとうございます！
助かりました

@taguchimasahiro taguchimasahiro added this to the 4.2.0 milestone Jul 27, 2022
@taguchimasahiro taguchimasahiro added enhancement 機能追加 security security labels Jul 27, 2022
@chihiro-adachi chihiro-adachi merged commit cefe54e into EC-CUBE:4.2 Aug 1, 2022
@chihiro-adachi
Copy link
Contributor

@pineray
ありがとうございます。マージしました。

chihiro-adachi さんのhttps://github.com/EC-CUBE/ec-cube/issues/4249#issuecomment-1189715241では .env にパラメーターを設定して、それを security.yaml へ渡すという仕様でしたが、そのようにすると max_attempts の値が not numeric value のエラーとなるため、security.yaml へ直接記述するようにしています。

こちら了解です。エラーになるのを確認しました。
環境変数での定義はできないということで、パラメータで定義するように修正追加しています。

This was referenced Aug 1, 2022
Closed
Closed
This was referenced Sep 13, 2022
Closed
Open
Merged
@chihiro-adachi chihiro-adachi changed the title Add a login throttling to firewalls. Add a login throttling to firewalls(アカウントロック、ログイン回数制限、ログイン試行回数制限) Sep 16, 2022
@chihiro-adachi chihiro-adachi mentioned this pull request Jul 11, 2023
Closed
@chihiro-adachi chihiro-adachi changed the title Add a login throttling to firewalls(アカウントロック、ログイン回数制限、ログイン試行回数制限) Add a login throttling to firewalls(アカウントロック、ログイン回数制限、ログイン試行回数制限、スロットリング) Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nanasess nanasess nanasess approved these changes

Assignees
No one assigned
Labels
enhancement 機能追加 security security
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@pineray @codecov-commenter @nanasess @chihiro-adachi @taguchimasahiro